Thanks for parsing the jslint output.
FWIW this is the very same JS code that is running in most opensocial
production instances today. To say it's not "production ready" just
not true. The truth is that this code fails to pass an arbitrary set
of guidelines. Try running PMD or findbugs on the Java code and
you'll get a simliar list of scary errors that need to be fixed right
now!
Also, jslint is just stupid in some sections. For example:
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/core/legacy.js:190:63:'escape' is undefined.
return window.encodeURIComponent ? encodeURIComponent(str) :
escape(str);
This is code that attempts to use encodeURIComponent on browsers that
support it and falls back to escape() for ancient browsers. This is
probably overkill since I doubt that OpenSocial performs very well on
Netscape Navigator or IE4 :)
In any case, I'll see about cleaning up some of these if it pushes
things along.
For the future it might be nice to enable the jslint maven plugin for
the release target. That combined with standard criteria might serve
us well for future revs.
On Dec 11, 2008, at 8:41 PM, Henning P. Schmiedehausen wrote:
As I mentioned before, I ran jslint in a very light setting on the
features javascript code. After applying the changes in SHINDIG-776 to
the tree, this leaves me with the following messages:
a) eval is evil:
While jslint flags these, I don't see a simple way to change
this. IMHO in these places, eval is probably the right answer. So no
reason to changes this unless someone comes up with a good solution to
this.
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/core.io/io.js:173:15:eval is evil.
var data = eval("(" + txt + ")");
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/core/auth.js:154:16:eval is evil.
trusted = eval("(" + config.trustedJson + ")");
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/core/json.js:159:15:eval is evil.
return eval('(' + text + ')');
^
b) undefined methods:
I have no ideas where those methods are and what they do. Typos?
Remnants?
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/core/legacy.js:190:63:'escape' is undefined.
return window.encodeURIComponent ? encodeURIComponent(str) :
escape(str);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/core/legacy.js:199:63:'unescape' is undefined.
return window.decodeURIComponent ? decodeURIComponent(str) :
unescape(str);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/core/util.js:119:67:'unescape' is undefined.
var unesc = window.decodeURIComponent ? decodeURIComponent :
unescape;
^
c) unclear breaks:
Some of these seem to be intentional fallthroughs but this is sloppy
code at best. Either use if elseif else or document clearly what the
code does.
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/flash/flash.js:81:58:Expected a 'break' statement before
'case'.
swfContainer = document.getElementById(swfContainer);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/flash/flash.js:85:6:Expected a 'break' statement before
'default'.
}
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/flash/flash.js:92:21:Expected a 'break' statement before
'case'.
opt_params = {};
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/views/views.js:155:22:Expected a 'break' statement before
'case'.
flag = 1;
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/views/views.js:181:22:Expected a 'break' statement before
'case'.
flag = 1;
^
d) scary for loops
The explanation on the jslint homepage is sufficient enough for me
that I don't like these:
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/flash/flash.js:151:8:The body of a for in should be wrapped
in an if statement to filter unwanted properties from the prototype.
for (var attrProp in attr) {
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/opensocial-base/jsonactivity.js:75:2:The body of a for in
should be wrapped in an if statement to filter unwanted properties
from the prototype.
for (var field in oldObject) {
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/pubsub/pubsub-router.js:65:10:The body of a for in should
be wrapped in an if statement to filter unwanted properties from the
prototype.
for (var subscriber in channelSubscribers) {
^
f) C'tor names
This is more coding style than anything else. However, most C'tors
adhere to this, only those fail. Yes, I understand that they probably
can't be fixed; however then this should be documented. Jslint is a
standard tool an people will run it on our code base, pointing these
things out again and again.
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/opensocial-base/jsonactivity.js:67:26:A constructor name
should start with an uppercase letter.
fieldValue[i] = new className(fieldValue[i]);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/opensocial-base/jsonperson.js:78:25:A constructor name
should start with an uppercase letter.
map[fieldName] = new className(fieldValue);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/opensocial-base/jsonperson.js:86:26:A constructor name
should start with an uppercase letter.
fieldValue[i] = new className(fieldValue[i]);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/opensocial-reference/opensocial.js:458:23:A constructor
name should start with an uppercase letter.
this.prototype = new tempCtor();
^
g) Line breaking errors
Javascript is not Java. Especially when it comes to
whitespace. Applying Java linebreaking to Javascript code might break
it in subtle ways so I prefer not to do it unless necessary. While
SHINDIG-766 removes most of the line length motivated line breaks, the
ones on rpc.js between 288 and 349 remain, making the whole VB program
flag up in red. This should be an one-liner with explanations on top.
h) Javascript URLs
There is a lot of discussion whether one should use javascript:void(0)
or simply '#' as anchor. However, Jslint flags these.
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/tabs/tabs.js:430:17:Script URL.
leftNav.href = 'javascript:void(0)';
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/tabs/tabs.js:451:18:Script URL.
rightNav.href = 'javascript:void(0)';
^
i) functions in loops
That code is a) unreadable and b) error prone. Needs fixing
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/views/views.js:162:19:Be careful when making functions
within a loop. Consider putting the function in a closure.
}).flag) {
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/
features/views/views.js:171:15:Be careful when making functions
within a loop. Consider putting the function in a closure.
}).join(arg));
IMHO, most of these things *should* be addressed before even thinking
to release this javascript code as "production ready". Either
documented as "this is intentional and a jslint exception around it"
or fixed. I prefer fixed.
Ciao
Henning
--
Henning P. Schmiedehausen - Palo Alto, California, U.S.A.
henn...@schmiedehausen.org "We're Germans and we use Unix.
henn...@apache.org That's a combination of two demographic
groups
known to have no sense of humour
whatsoever."
-- Hanno Mueller,
de.comp.os.unix.programming
Paul Lindner
plind...@hi5.com
